Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: makes existsSync stop using errors #24068

Closed
wants to merge 1 commit into from

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Nov 3, 2018

The previous implementation of existsSync was a try/catch on top
of accessSync. While conceptually sound it was a performance problem
when running it repeatedly on non-existing files, because accessSync
had to create an Error object that was immediatly discarded (because
existsSync never reports anything else than true / false).

This implementation simply checks whether the context would have caused
an exception to be thrown, but doesn't actually create it.

I benchmarked it as such:

const fs = require('fs');

console.time();
for (let t = 0; t < 100000; ++t) fs.existsSync('./doesnt-exist');
console.timeEnd();
❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ node --version
v11.1.0

❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ node test.js
default: 2543.314ms

❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ ./node test.js
default: 287.442ms

Fixes: #24008

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The previous implementation of `existsSync` was a try/catch on top
of `accessSync`. While conceptually sound it was a performance problem
when running it repeatedly on non-existing files, because `accessSync`
had to create an `Error` object that was immediatly discarded (because
`existsSync` never reports anything else than `true` / `false`).

This implementation simply checks whether the context would have caused
an exception to be thrown, but doesn't actually create it.

Fixes: nodejs#24008
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 3, 2018
@arcanis arcanis mentioned this pull request Nov 3, 2018
@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2018

I believe this change is already being made in #24015?

@arcanis
Copy link
Contributor Author

arcanis commented Nov 3, 2018

Oh, my bad! I see Github still doesn't send notifs when a PR is attached to an issue .. 😅

@arcanis arcanis closed this Nov 3, 2018
@refack
Copy link
Contributor

refack commented Nov 4, 2018

Oh, my bad!

I wouldn't worry about that. Hope to see you contribute more in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants